[azservicebus] Enable distributed tracing #23860

karenychen · 2024-12-11T01:21:34Z

The purpose of this PR is explained in this or a referenced issue.
The PR does not update generated files.
- These files are managed by the codegen framework at Azure/autorest.go.
Tests are included and/or updated for code changes.
Updates to module CHANGELOG.md are included.
MIT license headers are included in each file.

github-actions · 2024-12-11T01:21:50Z

Thank you for your contribution @karenychen! We will review the pull request and get back to you soon.

karenychen · 2024-12-11T01:23:45Z

sdk/messaging/azservicebus/internal/tracing/fake_tracing.go

Currently a couple of other packages have something similar to validate the spans in unit tests. We should move this to azcore/tracing so more libraries can use this fake trace provider in their tests?

It would probably go in the sdk/internal module so it can be internally shared but not publicly surfaced.

sdk/messaging/azservicebus/internal/constants.go

karenychen · 2024-12-11T19:26:22Z

Hi @lmolkova ! I had a small question regarding diagnostic-id in https://learn.microsoft.com/en-us/azure/service-bus-messaging/service-bus-end-to-end-tracing?tabs=net-standard-sdk-2

The .NET SDK seems to be hooking them up to a ReceiveMessages trace when the users set the diagnostic-id and linking the list of diagnostic ids from the messages to the Receive trace (code here). I might be misunderstanding how .NET is doing it, but I am wondering what we enable with the diagnostic-ids?

richardpark-msft

Looks great so far, got some questions for the other experts in the crowd, but from an SB perspective it looks great.

richardpark-msft · 2024-12-11T19:21:47Z

sdk/messaging/azservicebus/sender_unit_test.go

@@ -45,14 +46,45 @@ func TestSender_UserFacingError(t *testing.T) {

 	var asSBError *Error

-	err = sender.SendMessage(context.Background(), &Message{}, nil)
+	msgID := "testID"


We still want the original test ( err = sender.SendMessage(context.Background(), &Message{}, nil) here, but otherwise loving the addition of tracing to the mix.

sdk/messaging/azservicebus/client_test.go

richardpark-msft · 2024-12-11T19:30:45Z

sdk/messaging/azservicebus/client.go

+		return creds.fullyQualifiedNamespace
+	}
+
+	parts := strings.Split(creds.connectionString, "/")


We have an actual parser for the connection string, we should definitely use that. Look in internal/conn.

richardpark-msft · 2024-12-11T19:32:36Z

sdk/messaging/azservicebus/tracing.go

+}
+
+func getSpanAttributesForMessage(message *Message) []tracing.Attribute {
+	attrs := []tracing.Attribute{}


Suggested change

attrs := []tracing.Attribute{}

var attrs []tracing.Attribute

I swear there's some linter that complains if you pre-init the slice and it's not technically needed anyways.

richardpark-msft · 2024-12-11T19:33:59Z

sdk/messaging/azservicebus/sender.go

+	)
+	defer func() { endSpan(err) }()
+
+	err = s.links.Retry(ctx, EventSender, "SendMessageBatch", func(ctx context.Context, lwid *internal.LinksWithID, args *utils.RetryFnArgs) error {
 		return lwid.Sender.Send(ctx, batch.toAMQPMessage(), nil)


@lmolkova, in cases like this where I have retries, should the span reporting be in the retry loop, or outside, like we have here?

The way it works in our HTTP SDKs is that the method span is above the retry layer, and its child HTTP span is below the retry layer. So you'd have spans like this.

Some method call span HTTP span retry 1 HTTP span retry 2

I presume we'd want to do the same thing here.

@jhendrixMSFT I did a bit of digging -- the semantic conventions for HTTP has examples for how the spans should look like for retries: https://opentelemetry.io/docs/specs/semconv/http/http-spans/#http-client-authorization-retry-examples. However, for messaging systems there is not much information besides this chunk:

I am not sure if this implies the messaging spans are 1 per operation?

richardpark-msft · 2024-12-11T19:36:12Z

sdk/messaging/azservicebus/sender.go

+	ctx, endSpan := s.startSpan(ctx, "ScheduleAMQPAnnotatedMessages", tracing.ScheduleOperationName,
+		tracing.Attribute{Key: tracing.BatchMessageCount, Value: int64(len(messages))},
+	)
+	defer func() { endSpan(err) }()


@jhendrixMSFT, would it be worth building this pattern (via a callback, probably) into the tracing library? It can be internal, but it seems like everyone's going to do the "last error gets passed to endSpan before block ends" pattern.

If not, @karenychen, we can build a helper function - maybe we'd stick it right in the retry function to make things easier since we're passing very similar information to both.

Maybe it goes in the sdk/internal module?

Synced with Richard offline -- we are moving this to the Retry() layer :)

richardpark-msft · 2024-12-11T19:40:17Z

sdk/messaging/azservicebus/sender.go

+	spanName = fmt.Sprintf("Sender.%s", spanName)
+	attributes := []tracing.Attribute{


Maybe we should define these all the operation constants as 'Sender.ScheduledMessages' (ie, preformatted) and avoid the formatting operation/concat that we have to do here. That's easily for a future PR, I'm sure there's other spots I do the same thing.

Just to clarify, did we want to define the span names when we are calling the span, or did we want to have a set of pre-defined span names (similar to the operation type below) where we can directly grab and use?

"When calling" is fine, but having predefined 'const''s could be nice from a documentation perspective (ie: these are all the spans that exist).

(leaving it up to you, I'm good with either approach)

…rting a span

karenychen · 2024-12-12T18:57:04Z

sdk/messaging/azservicebus/liveTestHelpers_test.go

@@ -186,10 +187,11 @@ func deleteSubscription(t *testing.T, ac *admin.Client, topicName string, subscr
 // and fails tests otherwise.
 func peekSingleMessageForTest(t *testing.T, receiver *Receiver) *ReceivedMessage {
 	var msg *ReceivedMessage
+	// TODO


General question: is it possible for me to test the traces in outside of the local unit tests too? Are there instructions on how I can run the live tests (and potentially the stress tests)?

karenychen · 2024-12-12T19:33:32Z

sdk/messaging/azservicebus/sender.go

+	ctx, endSpan := s.startSpan(ctx, "ScheduleAMQPAnnotatedMessages", tracing.ScheduleOperationName,
+		tracing.Attribute{Key: tracing.BatchMessageCount, Value: int64(len(messages))},
+	)
+	defer func() { endSpan(err) }()


Synced with Richard offline -- we are moving this to the Retry() layer :)

karenychen · 2024-12-12T20:07:24Z

sdk/messaging/azservicebus/sender.go

+	)
+	defer func() { endSpan(err) }()
+
+	err = s.links.Retry(ctx, EventSender, "SendMessageBatch", func(ctx context.Context, lwid *internal.LinksWithID, args *utils.RetryFnArgs) error {
 		return lwid.Sender.Send(ctx, batch.toAMQPMessage(), nil)


@jhendrixMSFT I did a bit of digging -- the semantic conventions for HTTP has examples for how the spans should look like for retries: https://opentelemetry.io/docs/specs/semconv/http/http-spans/#http-client-authorization-retry-examples. However, for messaging systems there is not much information besides this chunk:

I am not sure if this implies the messaging spans are 1 per operation?

karenychen added 2 commits December 10, 2024 17:19

add internal tracing wrapper and fake tracer for UT

feffdf8

set up tracer in SB client and traces in sender methods

2f4a2b2

github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Service Bus labels Dec 11, 2024

karenychen commented Dec 11, 2024

View reviewed changes

karenychen added 2 commits December 10, 2024 17:57

add more unit tests

7c2e4ce

linting

23d7e94

richardpark-msft requested review from richardpark-msft, jhendrixMSFT and lmolkova December 11, 2024 19:19

jhendrixMSFT reviewed Dec 11, 2024

View reviewed changes

sdk/messaging/azservicebus/internal/constants.go Outdated Show resolved Hide resolved

richardpark-msft reviewed Dec 11, 2024

View reviewed changes

karenychen added 6 commits December 11, 2024 15:40

move matcher to sdk/internal folder and add callback function for sta…

1be63f7

…rting a span

address comments and moved startspan snippet to retrier layer

b026074

reverting some files

3d07a0a

added receiver traces and some UT

1bb68ed

add session traces

e653088

linting

992d9b9

karenychen commented Dec 12, 2024

View reviewed changes

reverting some files

af668bf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[azservicebus] Enable distributed tracing #23860

[azservicebus] Enable distributed tracing #23860

karenychen commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024

karenychen Dec 11, 2024

jhendrixMSFT Dec 11, 2024

karenychen commented Dec 11, 2024

richardpark-msft left a comment

richardpark-msft Dec 11, 2024

richardpark-msft Dec 11, 2024

richardpark-msft Dec 11, 2024

richardpark-msft Dec 11, 2024

jhendrixMSFT Dec 12, 2024

karenychen Dec 12, 2024

richardpark-msft Dec 11, 2024

richardpark-msft Dec 11, 2024

jhendrixMSFT Dec 11, 2024

karenychen Dec 12, 2024

richardpark-msft Dec 11, 2024

karenychen Dec 11, 2024 •

edited

Loading

richardpark-msft Dec 11, 2024

richardpark-msft Dec 11, 2024

karenychen Dec 12, 2024

karenychen Dec 12, 2024

karenychen Dec 12, 2024

		spanName = fmt.Sprintf("Sender.%s", spanName)
		attributes := []tracing.Attribute{

[azservicebus] Enable distributed tracing #23860

Are you sure you want to change the base?

[azservicebus] Enable distributed tracing #23860

Conversation

karenychen commented Dec 11, 2024 • edited Loading

github-actions bot commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenychen commented Dec 11, 2024

richardpark-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenychen Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenychen commented Dec 11, 2024 •

edited

Loading

karenychen Dec 11, 2024 •

edited

Loading